Skip to content

Conversation

@MarioCadenas
Copy link
Collaborator

πŸ“š Stack (3/3)

🏠 main (base)
β”‚
└──▢ βœ… refactor-move-core-plugins
     β”‚
     └──▢ βœ… plugin-manifest-definition
          β”‚
          └──▢ πŸ‘‰ resource-registry β—€ this PR

πŸ“ Changes

  • b7f9199 chore: resource registry

πŸ“– About this stack

This PR is part of a stack of changes. Each PR in the stack builds on the one below it.

  • ⬇️ Base: This PR is based on plugin-manifest-definition
  • ⬆️ Next: This is the top of the stack

Copy link
Member

@pkosiec pkosiec left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overal LGTM, two small comments - please check them and address them (if applicable) here or in a follow-up PR πŸ‘

@@ -15,7 +15,16 @@ interface ReconnectStreamResponse {

export class ReconnectPlugin extends Plugin {
public name = "reconnect";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BTW - do we need the public name for plugins if it's already in the static manifest? Or is it like an instance name which should vary when you instantiate the class multiple times?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we extract resource collection and validation to some separate methods? The logic here a bit makes it harder to read the high-level logic for registering plugins.

Like it was before:

    pluginInstance.validateEnv();

so now I can imagine, we'd have two calls: validation + collecting resources

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants